Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove other notification actions #64

Merged
merged 4 commits into from
May 29, 2024

Conversation

murilommen
Copy link
Contributor

details:

  • it has been quite a while since we only support GlobalActions
  • this PR removes both the helper methods to turn models into GlobalActions as well as typing references to other models
  • it might break existing customers using the tuned models, so the next release should communicate this properly
  • this PR also fixes a bunch of typing issues after running pyright on the repo

details:
- it has been quite a while since we only support GlobalActions
- this PR removes both the helper methods to turn models into GlobalActions
as well as typing references to other models
- it might break existing customers using the tuned models, so the next release
should communicate this properly
- this PR also fixes a bunch of typing issues after running pyright on the repo
@@ -217,7 +180,7 @@ class Monitor(NoExtrasBaseModel):
regex="[0-9a-zA-Z \\-_]+",
)
tags: Optional[ # type: ignore
List[constr(min_length=3, max_length=32, regex="[0-9a-zA-Z\\-_]")] # noqa F722
List[constr(min_length=3, max_length=32, regex="[0-9a-zA-Z\\-_]")] # type:ignore # noqa: F722
Copy link
Collaborator

@christinedraper christinedraper May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably fix the type error here and in analyzer by changing constr to Annotated like I did with monitor schema

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just caught an error by defining analyzerIds to be a str (same as you did for the monitor schema), so I believe we should also update that there!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try changing the List[constr... on tags? So maybe type:ignore can go away

@murilommen murilommen temporarily deployed to whylabs-toolkit-ci May 27, 2024 19:42 — with GitHub Actions Inactive
@christinedraper christinedraper self-requested a review May 29, 2024 15:21
@murilommen murilommen force-pushed the dev/murilommen/fix-remove-other-actions branch from eceb27f to bce2ebe Compare May 29, 2024 18:43
@murilommen murilommen force-pushed the dev/murilommen/fix-remove-other-actions branch from bce2ebe to 404a5fb Compare May 29, 2024 18:48
@murilommen murilommen temporarily deployed to whylabs-toolkit-ci May 29, 2024 18:48 — with GitHub Actions Inactive
@murilommen murilommen merged commit 40517b6 into mainline May 29, 2024
1 check passed
@murilommen murilommen deleted the dev/murilommen/fix-remove-other-actions branch May 29, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants